feat(swift-sdk): deleteWallet wipes full wallet footprint#3653
Draft
llbartekll wants to merge 3 commits into
Draft
feat(swift-sdk): deleteWallet wipes full wallet footprint#3653llbartekll wants to merge 3 commits into
llbartekll wants to merge 3 commits into
Conversation
`PlatformWalletManager.deleteWallet(walletId:)` wipes the wallet's Rust manager state, SwiftData rows (including orphans the `@Relationship` graph misses: `PersistentTransaction`, `PersistentPendingInput`, `PersistentTokenBalance`), identity-keyed and wallet-keyed Keychain entries, and the network sync state when no sibling wallets remain. Closes the data-leakage path where `modelContext.delete(wallet)` alone left orphan rows on the same network for the next wallet to see — the bug that prompted this on dashwallet-ios and was present in the SDK's own example app. Rust-side adds: - `platform_wallet_manager_remove_wallet` FFI (idempotent on missing). - `PlatformWalletPersistence::retire_wallet` trait method (default no-op). - `FFIPersister` retired set: drops non-registration `store`/`flush` for retired wallets and auto-clears on a `wallet_metadata`-bearing changeset so a same-session reimport recovers. - `PlatformWalletManager::remove_wallet` snapshots the wallet's identity ids and `IdentitySyncManager::unregister_identity`s each before dropping the wallet, so per-identity token sync stops. - `IdentitySyncManager::apply_fresh_balances` re-checks the live state under its write lock before persisting, closing the race where a mid-sync unregister could still emit a token balance. Swift-side adds: - `PlatformWalletPersistenceHandler.deleteWalletData` + companion `identityIdsForWallet` for the orchestration. - `KeychainManager.deleteAllKeychainItems(forIdentityId:)` (sweeps both `privkey_*` and `specialkey_*` schemes) and `deleteAllIdentityPrivateKeysForWallet(walletId:)` throwing variants so partial-failure surfaces instead of silently passing. Tests: - `WalletDeletionTests` (Swift, in-memory `ModelContainer`): cascade + orphan-tx sweep, idempotency, network-sync conditional cleanup (both branches), and per-scheme Keychain sweeps. - `retired_wallet_drops_non_registration_store_and_flush` and `registration_store_clears_retirement` (Rust): persister retirement semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the `do { try X; return true } catch { return false }` bridge
pattern around `deleteAllPrivateKeys(for:)` and
`deleteAllIdentityPrivateKeys(forWalletId:)`. The throwing variants
take over the canonical names; callers `try` directly. Drops the
parallel `*Throwing` / `*ForWallet` names.
Replaces seven inline `map { String(format: "%02x", $0) }.joined()`
hex-format duplications with the existing `Data.toHexString()`
extension, and removes the same-shape `hex(_:)` helper from
`WalletDeletionTests` in favor of the same extension.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request implements complete wallet deletion across the platform. The Rust backend introduces a retirement mechanism to gate stale persistence writes, coordinates identity unregistration with wallet removal, and robustifies identity syncing to handle unregistered identities. The Swift layer adds deletion APIs for SwiftData and Keychain, and orchestrates the full cleanup workflow including FFI calls and in-memory state updates. ChangesWallet Deletion Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "6a7d38f3ff70b54502868ce2a3de260b42bce1c295697faebb4d671f9942302c"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/manager/identity_sync.rs (1)
570-587:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid calling external persistence while holding
statewrite lock.
self.persister.store(...)runs underself.state.write(). Because this is an external call, it can stall or re-enter paths that needstate, blockingregister/unregister/update_watched_tokensand creating deadlock risk.Take a snapshot under lock, release lock before
store, then reacquire and re-check identity before applying cache updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/identity_sync.rs` around lines 570 - 587, Currently persister.store(...) is called while holding the self.state.write() lock which can deadlock; instead, inside identity_sync.rs (around the code using state.get(&identity_id), existing_row, sentinel, and cs) clone or take a snapshot of the changeset (cs) and any needed existing_row data while holding the write lock, then drop the lock before calling self.persister.store(sentinel, cs_snapshot), and after store returns reacquire the write lock and re-check that the identity still exists (state.get(&identity_id)) before applying any cache updates or mutations; ensure you handle and log persister.store errors the same way but performed outside the lock.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift (2)
149-191: ⚡ Quick winAdd a non-matching wallet control for the wallet-scoped key sweep.
This only proves that the targeted
walletIdrow is removed. BecausedeleteAllIdentityPrivateKeys(forWalletId:)enumerates the entire service, a broken filter that deletes everyidentity_privkey.*item would still satisfy this test. Seed a second row with a differentwalletIdand assert it survives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift` around lines 149 - 191, The test testThrowingKeychainSweepsUseIsolatedService currently only inserts one identity private key so deleteAllIdentityPrivateKeys(forWalletId:) could accidentally delete all identity_privkey.* rows; insert a second identity private key via KeychainManager.storeIdentityPrivateKey with a different walletId (e.g., walletId2 and publicKey2) before calling manager.deleteAllIdentityPrivateKeys(forWalletId: walletId) and after the deletion assert that retrieveIdentityPrivateKey(publicKeyHex: publicKey2) still returns non-nil while the original publicKey is nil, ensuring the sweep is scoped to the targeted walletId.
38-120: ⚡ Quick winExercise a transaction that becomes orphaned because of the wallet cascade.
orphanTxstarts out orphaned before deletion, so this test still passes ifdeleteWalletData(walletId:)misses transactions that only become orphaned after wallet/account/TXO rows are removed. Please add one wallet-owned TXO/transaction pair and assert that the transaction is swept after the delete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift` around lines 38 - 120, Add a transaction+TXO pair that is initially owned by the wallet and therefore removed by the wallet cascade: create a new PersistentTransaction named walletOwnedTx and a matching PersistentTxo named walletOwnedTxo that is associated with the wallet (set whichever field ties a txo to a wallet in your model—e.g., walletId or an address/ownership field) and append walletOwnedTxo to walletOwnedTx.outputs, insert both into the context alongside the existing objects before saving, then after calling PlatformWalletPersistenceHandler.deleteWalletData(walletId:) assert that the fetched PersistentTransaction list no longer contains walletOwnedTx (i.e., transactions count remains 1 and walletOwnedTx is gone). This ensures transactions that only become orphaned by the wallet cascade are swept.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 416-422: store() currently only checks the retired set once and
can persist changes if retire_wallet() runs concurrently, allowing stale writes
for just-retired wallets; modify the persistence path to coordinate retirement
and writes atomically by introducing a per-wallet lifecycle lock or state
machine (e.g., a per-wallet mutex/atomic state) and use it in both store() and
retire_wallet() so that store() acquires the wallet lock/state before running
the callback and verifies retirement state again (or aborts) before committing
pending merges; ensure you reference and protect the same retired set and the
code paths that examine changeset.wallet_metadata and perform pending merges so
no post-retire writes can be committed.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 349-353: The deleteWallet(walletId:) flow calls
persistenceHandler?.identityIdsForWallet(...) and proceeds to call
KeychainManager.shared.deleteAllIdentityPrivateKeys(forWalletId:) even when
persistenceHandler is nil (e.g., configure(..., modelContainer: nil)), which can
leave private keys in Keychain; update deleteWallet(walletId:) to guard that
persistenceHandler is present and identityIds were resolved before performing
any destructive Keychain operations—either make persistenceHandler mandatory for
this path (throw an error if nil) or explicitly fail early when identityIds
cannot be retrieved, using the existing persistenceHandler and
identityIdsForWallet(...) check to decide whether to proceed with
KeychainManager.shared.deleteAllKeychainItems(...) and
deleteAllIdentityPrivateKeys(...).
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- Around line 570-587: Currently persister.store(...) is called while holding
the self.state.write() lock which can deadlock; instead, inside identity_sync.rs
(around the code using state.get(&identity_id), existing_row, sentinel, and cs)
clone or take a snapshot of the changeset (cs) and any needed existing_row data
while holding the write lock, then drop the lock before calling
self.persister.store(sentinel, cs_snapshot), and after store returns reacquire
the write lock and re-check that the identity still exists
(state.get(&identity_id)) before applying any cache updates or mutations; ensure
you handle and log persister.store errors the same way but performed outside the
lock.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift`:
- Around line 149-191: The test testThrowingKeychainSweepsUseIsolatedService
currently only inserts one identity private key so
deleteAllIdentityPrivateKeys(forWalletId:) could accidentally delete all
identity_privkey.* rows; insert a second identity private key via
KeychainManager.storeIdentityPrivateKey with a different walletId (e.g.,
walletId2 and publicKey2) before calling
manager.deleteAllIdentityPrivateKeys(forWalletId: walletId) and after the
deletion assert that retrieveIdentityPrivateKey(publicKeyHex: publicKey2) still
returns non-nil while the original publicKey is nil, ensuring the sweep is
scoped to the targeted walletId.
- Around line 38-120: Add a transaction+TXO pair that is initially owned by the
wallet and therefore removed by the wallet cascade: create a new
PersistentTransaction named walletOwnedTx and a matching PersistentTxo named
walletOwnedTxo that is associated with the wallet (set whichever field ties a
txo to a wallet in your model—e.g., walletId or an address/ownership field) and
append walletOwnedTxo to walletOwnedTx.outputs, insert both into the context
alongside the existing objects before saving, then after calling
PlatformWalletPersistenceHandler.deleteWalletData(walletId:) assert that the
fetched PersistentTransaction list no longer contains walletOwnedTx (i.e.,
transactions count remains 1 and walletOwnedTx is gone). This ensures
transactions that only become orphaned by the wallet cascade are swept.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 755ba89b-4903-4bcd-a4b0-33c76ed16508
📒 Files selected for processing (11)
packages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/KeyManagerTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift
Comment on lines
+416
to
+422
| if wallet_id != WalletId::default() { | ||
| if changeset.wallet_metadata.is_some() { | ||
| self.retired.write().remove(&wallet_id); | ||
| } else if self.retired.read().contains(&wallet_id) { | ||
| return Ok(()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Retirement gate has a race window against concurrent retire_wallet.
store() only checks retired once up front, then runs callback persistence and pending merge later. If retire_wallet() runs between those points, stale writes can still be persisted for a just-retired wallet. This undermines the “drop post-delete writes” guarantee.
Consider a per-wallet lifecycle lock/state machine so retirement check + persistence path are coordinated atomically for non-registration writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-ffi/src/persistence.rs` around lines 416 - 422,
store() currently only checks the retired set once and can persist changes if
retire_wallet() runs concurrently, allowing stale writes for just-retired
wallets; modify the persistence path to coordinate retirement and writes
atomically by introducing a per-wallet lifecycle lock or state machine (e.g., a
per-wallet mutex/atomic state) and use it in both store() and retire_wallet() so
that store() acquires the wallet lock/state before running the callback and
verifies retirement state again (or aborts) before committing pending merges;
ensure you reference and protect the same retired set and the code paths that
examine changeset.wallet_metadata and perform pending merges so no post-retire
writes can be committed.
Comment on lines
+349
to
+353
| let identityIds = try persistenceHandler?.identityIdsForWallet(walletId: walletId) ?? [] | ||
| for identityId in identityIds { | ||
| try KeychainManager.shared.deleteAllKeychainItems(forIdentityId: identityId) | ||
| } | ||
| try KeychainManager.shared.deleteAllIdentityPrivateKeys(forWalletId: walletId) |
Contributor
There was a problem hiding this comment.
Guard the no-persistence path before claiming a full wipe.
configure(..., modelContainer: nil) is still a supported setup, but in that mode identityIds is always empty here, so privkey_* and specialkey_* rows are never swept. The method can therefore succeed while leaving identity key material behind in Keychain. Either make a persistence handler mandatory for deleteWallet(walletId:), or fail before the destructive FFI call when the identity list cannot be resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`
around lines 349 - 353, The deleteWallet(walletId:) flow calls
persistenceHandler?.identityIdsForWallet(...) and proceeds to call
KeychainManager.shared.deleteAllIdentityPrivateKeys(forWalletId:) even when
persistenceHandler is nil (e.g., configure(..., modelContainer: nil)), which can
leave private keys in Keychain; update deleteWallet(walletId:) to guard that
persistenceHandler is present and identityIds were resolved before performing
any destructive Keychain operations—either make persistenceHandler mandatory for
this path (throw an error if nil) or explicitly fail early when identityIds
cannot be retrieved, using the existing persistenceHandler and
identityIdsForWallet(...) check to decide whether to proceed with
KeychainManager.shared.deleteAllKeychainItems(...) and
deleteAllIdentityPrivateKeys(...).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
A wallet deletion through the SDK left orphan data behind: SwiftData rows the
@Relationshipcascade graph doesn't reach (PersistentTransaction,PersistentPendingInput,PersistentTokenBalance), in-memory Rust manager state, per-identity Keychain entries, and the shared network sync checkpoint. The next wallet created on the same network observed ghost rows from the deleted one. The same gap was present in the SDK's own example app — itsWalletDetailView.deleteWallet()carried aTODOacknowledging the missingPlatformWalletManagerremoval API.What was done?
Adds
PlatformWalletManager.deleteWallet(walletId:) throws, a single SDK call that wipes a wallet's complete footprint:platform_wallet_manager_remove_walletFFI drops the wallet from the in-memory map, snapshots its identities, and unregisters them fromIdentitySyncManagerso per-identity token-balance polling stops.retire_walletconcept — non-registration writes for retired wallet ids are dropped at the Rust→Swift boundary. Registration changesets auto-clear the retired flag, so re-importing the same seed works. This closes the race where Rust tasks already in flight when the delete fired could resurrect rows in SwiftData viaensureWalletRecord.IdentitySyncManager::apply_fresh_balancesre-checks the live state under its write lock before persisting, so a balance fetched mid-sync for a just-unregistered identity is dropped on the floor..nullify; this explicit path takes them down), sweepsPersistentPendingInputby walletId denorm, sweepsPersistentTransactionrows now reachable by nothing, sweepsPersistentTokenBalancefor the cascaded identity ids, and deletes the network sync state row only when no sibling wallet remains on that network.privkey_*+specialkey_*rows and wallet-derivedidentity_privkey.<path>rows, thenWalletStoragemetadata then mnemonic last (mnemonic-as-retry-anchor — if any earlier step fails, the pre-flight on retry still sees it).The example app's
WalletDetailViewbecomes the first consumer;dashwallet-iosfollows.How Has This Been Tested?
WalletDeletionTests(Swift, against in-memoryModelContainer): cascade + orphan-tx sweep, idempotency under double-call, network-sync conditional cleanup (both last-wallet and sibling-remains branches), and per-scheme Keychain sweeps with an isolated keychain service.rs-platform-wallet-ffi:retired_wallet_drops_non_registration_store_and_flushandregistration_store_clears_retirementpin the retire/auto-unretire semantics.cargo check -p platform-wallet,cargo clippy -p platform-wallet-fficlean.build_ios.sh --target sim --profile devsucceeds with-warnings-as-errors.WalletDeletionTestspass underxcodebuild test.Worth a simulator smoke pass before merging: create a wallet, sync a bit, delete it, create another on the same network, confirm StorageExplorerView shows zero ghosts for the deleted walletId across every
Persistent*table.Breaking Changes
KeychainManager.deleteAllPrivateKeys(for:)andKeychainManager.deleteAllIdentityPrivateKeys(forWalletId:)are nowthrows(were-> Boolpreviously, swallowing keychain errors). External consumers — includingdashwallet-ios— will need totrythe calls.Conventional-commit type is
feat, notfeat!, because the breaking surface is limited to two helper methods whose previousBoolreturn value didn't distinguish "nothing to delete" from "delete failed" — call sites that relied on the boolean were already silently wrong. Happy to retitle tofeat!if reviewers prefer.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Tests